Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(scripting/v8): raw event listeners #2194

Merged
merged 1 commit into from
Aug 20, 2024
Merged

fix(scripting/v8): raw event listeners #2194

merged 1 commit into from
Aug 20, 2024
+13 −9

Conversation

william-des
Copy link
Contributor

@william-des william-des commented Sep 8, 2023 •
edited
Loading

Goal of this PR

Fixes issue where raw event listeners weren't triggered without standard subscribers.
Skips payload deserialization when no standard event subscribers are present.

How is this PR achieving the goal

Fixing these issues in js scripting event function

This PR applies to the following area(s)

ScRT: JS

Successfully tested on

Game builds: 3095

Platforms: Windows, Linux

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

@thorium-cfx thorium-cfx self-assigned this Nov 3, 2023
@github-actions github-actions bot added the ScRT: JS Issues/PRs related to the JavaScript scripting runtime label Aug 6, 2024
@william-des william-des changed the title tweak(scripting/v8): skip unnecessary payload deserialization fix(scripting/v8): raw event listeners Aug 6, 2024
@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Aug 6, 2024
@william-des
fix(scripting/v8): raw event listeners
5adfa2f 
Raw event listerners were never triggered when no standard event listerners existed for the same event
FabianTerhorst
FabianTerhorst approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@FabianTerhorst FabianTerhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works fine 👍

@FabianTerhorst FabianTerhorst added ready-to-merge This PR is enqueued for merging and removed triage Needs a preliminary assessment to determine the urgency and required action labels Aug 16, 2024
@prikolium-cfx prikolium-cfx merged commit b6d2585 into citizenfx:master Aug 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-wide c-wide c-wide left review comments

@thorium-cfx thorium-cfx thorium-cfx requested changes

@FabianTerhorst FabianTerhorst FabianTerhorst approved these changes

@nihonium-cfx nihonium-cfx Awaiting requested review from nihonium-cfx

Assignees

@FabianTerhorst FabianTerhorst

Labels
ready-to-merge This PR is enqueued for merging ScRT: JS Issues/PRs related to the JavaScript scripting runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@william-des @FabianTerhorst @c-wide @thorium-cfx @prikolium-cfx